Size Change: +2.01 kB (0%)

Total Size: 1.44 MB

_{compressed-size-action}

👋 @diegohaz while I was working on this with @ciampo we actually found we had a question for you! A number of our unit tests began failing when we implemented Ariakit, and it turned out that the elements or text the test checked for didn't render immediately. The tests began passing when we updated them to run async with findby instead of getby.

We suspect this is due to re-renders caused by the different useEffect/useLayoutEffect calls we needed to preserve, but were curious if had any thoughts on a better approach. For this PR, (similar to the previous Radix attempt) it's important that we preserve all the functionality and behavior of the existing component. That'll be less critical in future PRs as we build out a more composable version.

Flaky tests detected in d938bd3.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5672378046
📝 Reported issues:

• [Flaky Test] should not allow blocks to be inserted in inner blocks #44341 in specs/editor/plugins/cpt-locking.test.js

👋 @diegohaz while I was working on this with @ciampo we actually found we had a question for you! A number of our unit tests began failing when we implemented Ariakit, and it turned out that the elements or text the test checked for didn't render immediately. The tests began passing when we updated them to run async with findby instead of getby.

We suspect this is due to re-renders caused by the different useEffect/useLayoutEffect calls we needed to preserve, but were curious if had any thoughts on a better approach. For this PR, (similar to the previous Radix attempt) it's important that we preserve all the functionality and behavior of the existing component. That'll be less critical in future PRs as we build out a more composable version.

If you don't provide defaultSelectedId, Ariakit will automatically select the first tab. For performance reasons, this happens in a requestAnimationFrame callback, which is not automatically flushed by JSDOM/React Testing Library.

In theory, you could do something like this right after render() or rerender():

await act(async () => {
  await new Promise(requestAnimationFrame);
});

This would flush the pending animation frame callbacks triggered after render and let you use getBy functions.

But this is an implementation detail. There may be cases where requestAnimationFrame, queueMicrotask, or even requestIdleCallback are used (when available) at different times. These are all flushed automatically by browsers but not by JSDOM/RTL.

In Ariakit, we use an internal testing package that automatically flushes those timers in JSDOM in a similar way that browsers would do, so our JSDOM tests don't include implementation details.

I'd say findBy is okay as a temporary solution. But I'd highly recommend using a real browser to test those components, especially the ones that rely on third-party libraries. Playwright works really well for this. As I said, you may find other cases where you'll have to manually flush timers between actions. Also, Ariakit (and other libraries) may introduce non-breaking changes that add those timers as an implementation detail, which could make those JSDOM tests fail.

I'd say findBy is okay as a temporary solution. But I'd highly recommend using a real browser to test those components, especially the ones that rely on third-party libraries. Playwright works really well for this.

Excellent, thanks so much for all of that insight @diegohaz!

A bit more open to debate is what happens when there's a focusable element (like a button) in the tab content. The ARIA guidance is somewhat unclear, and there's variance in the main library implementations. Ariakit and Radix still retain the tab stop on tabpanel, while React Aria will go straight to the focusable element.

Good catch! Ariakit should only focus on the tab panel element if there are no tabbable elements inside it. But it's currently not able to identify tabbable elements inside the tab panel after the first render, so it only works for the tab panel that's initially visible (using the defaultSelectedId prop). This should be addressed in future versions.

If that's a requirement now, passing tabIndex={-1} or focusable={false} to TabPanel (if there are tabbable elements inside it) should do it.

Thanks for all of the time and advice on this one @mirka. Final round of changes addressed, just waiting on CI 🚀

Seems that this PR caused that some unit tests started failing intermittently, because a button element is sometimes rendered with tabindex attribute, sometimes not.

 FAIL  packages/edit-post/src/components/preferences-modal/test/index.js
  ● EditPostPreferencesModal › should match snapshot when the modal is active › large viewports

    expect(received).toMatchSnapshot()

    Snapshot name: `EditPostPreferencesModal should match snapshot when the modal is active large viewports 1`

    - Snapshot  - 0
    + Received  + 2

    @@ -125,10 +125,11 @@
                  aria-selected="false"
                  class="components-button components-tab-panel__tabs-item"
                  data-command=""
                  id="tab-panel-0-blocks"
                  role="tab"
    +             tabindex="-1"
                  type="button"
                >
                  Blocks
                </button>
                <button
    @@ -136,10 +137,11 @@
                  aria-selected="false"
                  class="components-button components-tab-panel__tabs-item"
                  data-command=""
                  id="tab-panel-0-panels"
                  role="tab"
    +             tabindex="-1"
                  type="button"
                >
                  Panels
                </button>
              </div>

      27 | 			expect(
      28 | 				await screen.findByRole( 'dialog', { name: 'Preferences' } )
    > 29 | 			).toMatchSnapshot();
         | 			  ^
      30 | 		} );
      31 | 		it( 'small viewports', async () => {
      32 | 			useSelect.mockImplementation( () => [ true, true, false ] );

      at Object.toMatchSnapshot (packages/edit-post/src/components/preferences-modal/test/index.js:29:6)

It's a test that uses React Testing Library and JSDOM. @diegohaz is it possible that the tabindex prop is not present on initial render, but added only after on a second render triggered by some useEffect? Then we'd have to use .findBy or waitFor to wait until things settle down.

It's a test that uses React Testing Library and JSDOM. @diegohaz is it possible that the tabindex prop is not present on initial render, but added only after on a second render triggered by some useEffect? Then we'd have to use .findBy or waitFor to wait until things settle down.

Yes, tabindex={-1} is added only after hydration to allow keyboard users to navigate through tabs using the Tab key before JavaScript can manage arrow keys. Unfortunately, it's a tradeoff when testing these components with an emulated DOM. I'm developing testing utilities that automatically handle all these timers, but they're not quite ready for public use yet. However, we've been using them to test components within Ariakit for years. You can see an example here: https://github.com/ariakit/ariakit/blob/0eacd26eb42fb5c54208dc31b71ef675306f940f/examples/tab/test.ts.

Yes, tabindex={-1} is added only after hydration to allow keyboard users to navigate through tabs using the Tab key before JavaScript can manage arrow keys.

Is it the disableFocus function that does this? By setting the tabindex attribute directly on the DOM element, altering markup that should be owned by React?

And how is it scheduled relative to the initial render? Is there useEffect, or requestAnimationFrame, or setTimeout?

I'm developing testing utilities that automatically handle all these timers, but they're not quite ready for public use yet.

I see, there are utilities that do await act(), and creating a promise that resolves after a microtask tick, or after rAF, or after timeout... But that's an antipattern we're trying to avoid in Gutenberg, because it requires too much implementation knowledge from the test author.

The preferred way is to use findBy or waitFor to detect state after all scheduled tasks are flushed. That's how we'll need to fix the failing EditPostPreferencesModal.

Is it the disableFocus function that does this? By setting the tabindex attribute directly on the DOM element, altering markup that should be owned by React?

And how is it scheduled relative to the initial render? Is there useEffect, or requestAnimationFrame, or setTimeout?

tabIndex is not set directly. It's managed by React. It uses requestAnimationFrame, but this is an implementation detail.

I see, there are utilities that do await act(), and creating a promise that resolves after a microtask tick, or after rAF, or after timeout... But that's an antipattern we're trying to avoid in Gutenberg, because it requires too much implementation knowledge from the test author.

I'm not sure what utilities you're talking about, but the functions we use are like await press.Tab(), await press.ArrowLeft(), await click(). Internally, those functions mimic what the browser does, including flushing microtasks between related events, animation frames, and timeouts when necessary to replicate an actual browser behavior (for example, unlike tap(), a real click should have a timeout between mouse/pointer down and mouse/pointer up). The fact that it handles the implementation details in the library is just a consequence of mimicking the browser behavior.

This is the opposite of implementation knowledge. Unless you're talking about browser implementation knowledge, which we had to study a lot to build those functions, but that's entirely abstracted into the testing library.

The preferred way is to use findBy or waitFor to detect state after all scheduled tasks are flushed. That's how we'll need to fix the failing EditPostPreferencesModal.

Since @ariakit/test is not ready for public use, I usually recommend using Playwright because it'll guarantee tests that don't use findBy or waitFor today won't fail if Ariakit or another UI library changes the implementation. Also, sometimes it's not obvious what you need to waitFor, or you'll end up waiting for some unrelated state just to get rid of an act warning or something else.

But I understand Playwright may be slower and less convenient than JSDOM. So that's an okay solution, I guess.

tabIndex is not set directly. It's managed by React.

Yes, I found the relevant code in the useCompositeItem hook 👍

the functions we use are like await press.Tab(), await press.ArrowLeft(), await click(). Internally, those functions mimic what the browser does,

Yes, I was talking about the utility functions that the helpers like press.Tab() use.

If it was only about mimicking browser behavior, that would be fine. But in that case Ariakit wouldn't need its own helpers, the helpers in @testing-library/user-event already mimick the browser behavior very well, including timeouts between the simulated events etc.

But unfortunately there is more. A call like:

await act( () => new Promise( r => requestAnimationFrame( r ) ) );

isn't there just to mimick browser behavior. If it was, it wouldn't need the React act() call. It's there mainly to prevent act() warnings on code like this:

function TabButton() {
  const [ tabIndex, setTabIndex ] = useState( undefined );
  useEffect( () => {
    requestAnimationFrame( () => {
      setTabIndex( -1 );
    } );
  }, [] );

  return <button tabIndex={ tabIndex } />;
}

If you didn't call that await act( requestAnimationFrame ), you'd get an act() warning, because that delayed setTabIndex call is scheduled asynchronously and executed outside the act environment's control.

So, when user is testing their React component that uses some library that somewhere deep down does the useEffect + rAF + setState combo, the deep implementation detail leaks very visibly out of the library into the user's test. Because it's the user's test that must do a matching await act() call.

We've seen many instances of that when migrating Gutenberg to React 18 and newer versions of Testing Library. floating-ui has async effects that leak out, and now Ariakit has them, too.

The solution is to:

• carefully figure out what user-visible change does that async effect produce
• wait for that user-visible change in the unit test

If it was only about mimicking browser behavior, that would be fine. But in that case Ariakit wouldn't need its own helpers, the helpers in @testing-library/user-event already mimick the browser behavior very well, including timeouts between the simulated events etc.

The act function is specifically there to replicate browser behavior when using React, where effects are naturally flushed on timers. @ariakit/test doesn't have specific knowledge about @ariakit/react, floating-ui, framer-motion, react-router, and so on. However, it effectively works with all these libraries.

When I last experimented with user-event, it failed to correctly flush microtasks between events. This was the main reason we couldn't use it with Ariakit. The absence of act wasn't the only issue. This was years ago, and I see that it's greatly improved now. Nevertheless, to most accurately replicate browser behavior in a React application, it should either invoke act on timers or at least allow users to wrap their internal timers with a custom function. This way, users can pass React's act to it.

So, when user is testing their React component that uses some library that somewhere deep down does the useEffect + rAF + setState combo, the deep implementation detail leaks very visibly out of the library into the user's test. Because it's the user's test that must do a matching await act() call.

I respectfully disagree with your last statement. In almost all cases, a rAF within an effect or a queued microtask within an event handler should not leak into the user's test. If it does, this should not be deemed as a preferred solution.

Take your example, if a library invokes setTabIndex(-1), and then starts embedding it in rAF in a minor or patch version, your tests should not fail. As a user, you shouldn't have to spend time deciphering this and waitFor something just because of that. It's not required in the browser, it's not necessary with Playwright, and it's not needed with @ariakit/test. That's essentially what I'm trying to convey.

I realize that the current state of unit testing doesn't provide many solutions to this issue. But that is precisely what @ariakit/test aims to address. It's the complete opposite of "unfortunate."

if a library invokes setTabIndex(-1), and then starts embedding it in rAF in a minor or patch version, your tests should not fail.

The tests shouldn't fail, but, if they wait for specific fine-grained events inside the event loop, they will fail. It's very likely that if Ariakit's internal implementation changed, the @ariakit/test/click implementation would also have to change: the carefully placed await queuedMicrotasks() and await sleep() calls would need to placed differently. Because the individual React updates are now scheduled differently in the event loop.

Nevertheless, to most accurately replicate browser behavior in a React application, it should either invoke act on timers or at least allow users to wrap their internal timers with a custom function.

To really replicate accurate browser behavior in a React application, the tests would simply need to stop using act. Because the entire point of act is to disable the "native" scheduling of React updates, and instead schedule the updates on an artificial "act" queue. This queue can then be reliably flushed after the act callback returns. This lets us write synchronous tests for React components, even for code that runs useEffect, which is natively asynchronous.

Recent versions of React Testing Library completely disable the act environment when waitFor is active. Because that's the only way how to reliably run all the async updates the same way how browser would run them. To get Playwright style unit tests, Testing Library would need to run React without act, and do all checks asynchronously, i.e., with findBy. getBy becomes obsolete because we no longer can guarantee that anything happens sync.

The tests shouldn't fail, but, if they wait for specific fine-grained events inside the event loop, they will fail. It's very likely that if Ariakit's internal implementation changed, the @ariakit/test/click implementation would also have to change: the carefully placed await queuedMicrotasks() and await sleep() calls would need to placed differently. Because the individual React updates are now scheduled differently in the event loop.

queuedMicrotasks and sleep are carefully placed to mimic a real user action in the browser, where microtasks are flushed after individual dispatched events and some tasks might have a user delay between them. So, no, if Ariakit's internal implementation changes, the implementation of @ariakit/test shouldn't change. The only reason to modify @ariakit/test is if it's not executing actions the same way they would occur in the browser.

Recent versions of React Testing Library completely disable the act environment when waitFor is active. Because that's the only way how to reliably run all the async updates the same way how browser would run them. To get Playwright style unit tests, Testing Library would need to run React without act, and do all checks asynchronously, i.e., with findBy. getBy becomes obsolete because we no longer can guarantee that anything happens sync.

I wasn't aware you could completely disable the act environment. I'll take a look into that. We can definitely remove act from queuedMicrotasks and sleep if it's possible. Do you know if it also works with React 17?

I wasn't aware you could completely disable the act environment. [...] Do you know if it also works with React 17?

It's a matter of setting the global.IS_REACT_ACT_ENVIRONMENT to false, and not calling act. It's supported only since React 18, and is described very well in this discussion: reactwg/react-18#102. The "act is not the right API for all tests" section therein is particularly relevant.

I think that React Testing Library wants to maintain full control over the IS_REACT_ACT_ENVIRONMENT value, and it won't be easy to wrestle it away and set it reliably yourself. Might require some patches to support that.

Once you disable the act environment, React rendering and effects become fully async, scheduled with setImmediate, and you always need to poll until the expected DOM appears on the screen.

It's a matter of setting the global.IS_REACT_ACT_ENVIRONMENT to false, and not calling act. It's supported only since React 18, and is described very well in this discussion: reactwg/react-18#102. The "act is not the right API for all tests" section therein is particularly relevant.

I think that React Testing Library wants to maintain full control over the IS_REACT_ACT_ENVIRONMENT value, and it won't be easy to wrestle it away and set it reliably yourself. Might require some patches to support that.

Once you disable the act environment, React rendering and effects become fully async, scheduled with setImmediate, and you always need to poll until the expected DOM appears on the screen.

Thanks! I tried setting this global value to false in beforeAll and removed all act from @ariakit/test. Interestingly, 2 out of 400 tests failed on @ariakit/react, but those might have been using getBy or something similar. So, it seems we can certainly do away with act.

See #48440 for the combined dev note